-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Asana log message #6
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 comment for now
@@ -25,10 +24,11 @@ def self.run(params) | |||
task_id = Fastlane::Actions::AsanaExtractTaskIdAction.run(task_url: task_url) if task_url | |||
|
|||
if template_name.to_s.empty? | |||
text = "#{comment}\n\nWorkflow URL: #{workflow_url}" | |||
text = "#{comment}\n\nWorkflow URL: #{ENV.fetch('WORKFLOW_URL')}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please mind that ENV.fetch('WORKFLOW_URL')
will raise KeyError when a given variable isn't present. To prevent that, provide a default value that will be used when the variable is not found, e.g. ENV.fetch('WORKFLOW_URL', '')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work @jaceklyp! Leaving a bunch of minor comments, to simplify or clarify things. Other than that I love it. Thanks!
begin | ||
asana_client.tasks.add_followers_for_task(task_gid: automation_subtask_id, followers: [assignee_id]) | ||
rescue StandardError => e | ||
UI.user_error!("Failed to add a collaborator to the release task: #{e}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically it's not the release task that the user is added to, but the 'Automation' subtask of a release task. How about the following? To allow easier debugging, perhaps.
UI.user_error!("Failed to add a collaborator to the release task: #{e}") | |
UI.user_error!("Failed to add user #{assignee_id} as collaborator on task #{automation_subtask_id}: #{e}") |
description: "Github user handle", | ||
optional: true, | ||
type: String), | ||
FastlaneCore::ConfigItem.new(key: :is_scheduled_release, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConfigItem
provides default_value
parameter which you could leverage here instead of using || false
in the action body 👍
def self.load_template_file(template_file) | ||
File.read(template_file) | ||
rescue StandardError | ||
UI.user_error!("Error: The file '#{template_file}' does not exist.") | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious if we's ever need to call path_for_asset_file
standalone. If not, we could perhaps combine these two functions int one and have a function that computes the path and reads the file. And then write tests for it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes perfect sense, but let me do it in another PR today!
spec/asana_log_message_spec.rb
Outdated
test_action(task_url: task_url, comment: comment, is_scheduled_release: true) | ||
end | ||
|
||
it "does takes assignee id from github handle when is manual release" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/does takes/takes/
spec/asana_log_message_spec.rb
Outdated
test_action(task_url: task_url, comment: comment, is_scheduled_release: false, github_handle: "") | ||
end | ||
|
||
it "adds a assignee as a follower to the automation task" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adds an assignee as follower 😇
spec/asana_log_message_spec.rb
Outdated
allow(Fastlane::Actions::AsanaAddCommentAction).to receive(:run) | ||
end | ||
|
||
it "does extract assignee id from release task when is scheduled release" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extracts in place of does extract, maybe?
Task URL: https://app.asana.com/0/1201392122292466/1208137627434494/f
Tested on: https://app.asana.com/0/1201392122292466/1208233977568871/f